Ensure that Task SDK supervisor closes all its subprocess handles correctly. #44263
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ashb
added
the
area:task-execution-interface-aip72
ashb
force-pushed
the
fix-tasksdk-subprocess-closing
branch
from
8ceb4dd to
3cca463
Compare
ashb
changed the title
kaxil
approved these changes
Nov 21, 2024
If we don't we keep any copies of it open then the selector loop in monitor process will get stuck waiting on a read loop. Sadly I wasn't able to reproduce this behaviour in unit tests, but only when running this code for real. (The main fix here is to pass `child_stderr` to _close_unused_sockets too, the rest are drive-by tidy ups) Since we never access `proc.stdout` or stderr on the class (they are closed over in the callbacks only) I removed those properties as they aren't needed and shouldn't be accessed directly as it would lead to garbled output. Also add tests (and re-fix) the last-chance exception handling
ashb
force-pushed
the
fix-tasksdk-subprocess-closing
branch
from
e07dd99 to
2b14e69
Compare
LefterisXefteris
pushed a commit
to LefterisXefteris/airflow
that referenced
this pull request
Jan 5, 2025
…ache#44263) If we don't we keep any copies of it open then the selector loop in monitor process will get stuck waiting on a read loop. Sadly I wasn't able to reproduce this behaviour in unit tests, but only when running this code for real. (The main fix here is to pass `child_stderr` to _close_unused_sockets too, the rest are drive-by tidy ups) Since we never access `proc.stdout` or stderr on the class (they are closed over in the callbacks only) I removed those properties as they aren't needed and shouldn't be accessed directly as it would lead to garbled output. Also add tests (and re-fix) the last-chance exception handling
got686-yandex
pushed a commit
to got686-yandex/airflow
that referenced
this pull request
Jan 30, 2025
…ache#44263) If we don't we keep any copies of it open then the selector loop in monitor process will get stuck waiting on a read loop. Sadly I wasn't able to reproduce this behaviour in unit tests, but only when running this code for real. (The main fix here is to pass `child_stderr` to _close_unused_sockets too, the rest are drive-by tidy ups) Since we never access `proc.stdout` or stderr on the class (they are closed over in the callbacks only) I removed those properties as they aren't needed and shouldn't be accessed directly as it would lead to garbled output. Also add tests (and re-fix) the last-chance exception handling
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If we keep any copies of the handles open then the selector loop in monitor
process will get stuck waiting on a read loop on a socket that never closes
(because its open in the same process still!).
Sadly I wasn't able to reproduce this behaviour in unit tests, but only when
running this code for real. (The main fix here is to pass
child_stderrto_close_unused_sockets too, the rest are drive-by tidy ups)
Since we never access
proc.stdoutor stderr on the class (they are closedover in the callbacks only) I removed those properties as they aren't needed
and shouldn't be accessed directly as it would lead to garbled output.
Also add tests (and re-fix) the last-chance exception handling
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.